fix: prevent file handle leak when maxFiles is exceeded#1068
fix: prevent file handle leak when maxFiles is exceeded#1068guoyangzhen wants to merge 1 commit intonode-formidable:masterfrom
Conversation
Fixes node-formidable#987 When maxFiles limit is reached, the fileBegin event handler calls _error(), but _handlePart continues and opens a write stream for the new file. These file handles are never closed. Fix: check this.error after emitting fileBegin and before file.open(). If an error occurred (e.g., maxFiles exceeded), decrement _flushing and return early to prevent the file stream from being opened.
There was a problem hiding this comment.
Pull request overview
Fixes a resource-leak edge case in IncomingForm._handlePart() where exceeding options.maxFiles could trigger _error() during fileBegin, but _handlePart() would still proceed to file.open(), creating an untracked file handle.
Changes:
- Add an early-return guard after emitting
fileBeginto stop processing if an error was raised (e.g.,maxFilesexceeded). - Decrement
_flushingon this early-return path to keep the internal “in-flight” counter balanced.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Check for error after fileBegin (e.g., maxFiles exceeded) to avoid leaking file handles | ||
| if (this.error) { | ||
| this._flushing -= 1; |
There was a problem hiding this comment.
This fixes the multipart path, but the same maxFiles/fileBegin sequencing exists in the octet-stream plugin (src/plugins/octetstream.js): it emits fileBegin before file.open() and openedFiles.push(file). If maxFiles is exceeded there, _error() will run before the file is tracked and the handle can still leak. Consider applying the same this.error short-circuit (and matching _flushing handling) in the octet-stream code path as well to make the fix comprehensive.
| this._flushing -= 1; |
| // Check for error after fileBegin (e.g., maxFiles exceeded) to avoid leaking file handles | ||
| if (this.error) { | ||
| this._flushing -= 1; | ||
| return; | ||
| } |
There was a problem hiding this comment.
There are existing Jest tests asserting maxFiles emits an error, but they don’t cover the new behavior that prevents file.open()/handle creation after fileBegin triggers _error(). Please add a test that reproduces the leak scenario (e.g., stub _newFile() to return a file with an open spy and ensure open is not called for the part that exceeds maxFiles, and/or assert no streams remain open).
Fixes #987
Problem
When
options.maxFilesis set and a request exceeds the limit, file handles are leaked and never released until the process exits.Root cause in
_handlePart:this.emit('fileBegin', part.name, file)— triggers the maxFiles checkthis._error(), destroying files inthis.openedFiles_handlePartcontinues and callsfile.open()+this.openedFiles.push(file)Fix
Check
this.errorafter emittingfileBeginbut beforefile.open(). If an error occurred (e.g., maxFiles exceeded), decrement_flushingand return early to prevent opening the file stream.Reproduction
With the fix, no file handles are leaked when maxFiles is exceeded.